Notify listeners when days off are removed, and add the missing removal methods by Natalie-the-technician · Pull Request #2830 · bardsoftware/ganttproject

Natalie-the-technician · 2026-08-31T11:40:57Z

What is wrong

HumanResource.addDaysOff() has no counterpart. The only way to take an absence
away is to reach into the DefaultListModel that getDaysOff() hands out and
mutate it directly, and nothing observes that model. Adding an absence and
removing one are therefore not the same kind of operation: adding resets the
cached LoadDistribution and fires fireResourceChanged(), removing does
neither.

Steps to reproduce

Create a resource and give it exactly one day off. Press OK.
Open the Resources Chart. The absence is drawn as a yellow block.
Open the resource again, delete the day off, press OK.

Expected: the block disappears, and anything listening to the resource model is
told that something changed.

Actual: the block stays, and no resourceChanged event is fired at all.

Cause

GanttDialogPerson.applyChanges() clears the list before writing the edited
intervals back:

person.getDaysOff().clear();
for (DateInterval interval : myDaysOffModel.getIntervals()) {
  person.addDaysOff(new GanttDaysOff(interval.getStart(), interval.getEnd()));
}

With at least one interval left, the last addDaysOff() repairs both by
accident. Delete the last absence and the loop body never runs, so nothing
calls resetLoads() and nothing fires. resetLoads() drops the memoised
myLoadDistribution, which is the only cache of the computed absences — that is
why the chart keeps drawing the old picture.

That dialog line is the symptom rather than the defect. A fix that only makes
that one line fire leaves the getter handing out an unobserved mutable model,
and the next caller that removes an absence — a batch edit, an importer, a
script — reintroduces the same gap.

What this changes

1. getDaysOff() hands out an unmodifiable List view, and every change goes
through a method. The field is a plain ArrayList<GanttDaysOff> and the getter
returns Collections.unmodifiableList over it, typed List<GanttDaysOff> —
DefaultListModel is a Swing class and models a UI control, not a resource. A
view rather than a copy, so a caller holding on to it keeps seeing the resource.

addDaysOff(), removeDaysOff() and clearDaysOff() each call
onDaysOffChanged(), which does resetLoads() and fireResourceChanged().
The two halves depend on each other: a plain List cannot be observed, so the
notification has to live in the methods, and that is only sound because no
caller can reach past them any more.

removeDaysOff() fires only when something was really removed, and
clearDaysOff() returns early on an already empty list — what
DefaultListModel.removeElement() and clear() did by themselves.

The copying constructor fills the list directly with addAll() rather than
going through addDaysOff(), so copying stays silent without depending on
areEventsEnabled for the days off at all.

2. The two missing counterparts, in the shape HumanResourceManager already
uses for its own list (add / remove / clear):

boolean removeDaysOff(GanttDaysOff gdo)
void clearDaysOff()

removeDaysOff returns whether anything was removed, which is also the answer to
"did the listeners hear about it" — nothing fires when the interval was not
there. It is clearDaysOff rather than a bare clear() because a resource holds
assignments and custom properties too.

One caveat, documented on the method and pinned by a test rather than changed
here: removeDaysOff matches by Object.equals, the way the list matches.
GanttDaysOff only overloads equals(GanttDaysOff) and does not override
equals(Object), so an interval built afresh from the same two dates is not the
one the resource holds. Callers should pass an instance obtained from the
resource.

GanttDialogPerson.applyChanges() now calls clearDaysOff() instead of
getDaysOff().clear(), and no production code can write to the handed-out list
any more. The readers of getDaysOff() move from getSize()/getElementAt(i)
to size()/get(i); LoadDistribution and ProjectFileExporter were reading
through a raw DefaultListModel and lose their casts.

The number of notifications does not change

Clearing and then writing N intervals back still costs (M > 0 ? 1 : 0) + N
resourceChanged events, where M is the number of intervals before OK: one event
for the whole removal, none at all when there was nothing to remove. With a plain
List that is not free — List.clear() has no notion of firing — so
clearDaysOff() returns early on an empty list and removeDaysOff() returns
false without firing when nothing matched. The single event this PR adds is
exactly the one that was missing in the "delete the last absence" case.

Tests

HumanResourceDaysOffTest

removing the last day off notifies the listeners just as adding it does

HumanResourceRemoveDaysOffTest

removing a single day off takes it off the resource and notifies once
removing a day off the resource does not have changes nothing and notifies nobody
clearing the days off empties the list and notifies once, whatever the count
clearing an empty list of days off notifies nobody
the dialog's clear-all-and-rewrite still costs the same notifications —
walks every combination of M and N in 0..3, pins the absolute event counts
for clearDaysOff() so that an implementation firing once per interval would
be caught, and pins from the other side that reaching into the handed-out list
is rejected, removes nothing and notifies nobody.

HumanResourceDaysOffViewTest

the list handed out by getDaysOff cannot be modified — clear, add,
removeAt and set on the returned list all throw
UnsupportedOperationException, and the resource is unchanged afterwards.
the list handed out by getDaysOff is a view and not a copy — a day off added
after the getter call shows up in the list fetched before it.

Without the notification the first test fails with expected: <1> but was: <0>,
and without the unmodifiable view the first test of HumanResourceDaysOffViewTest
fails with Expected java.lang.UnsupportedOperationException to be thrown, but nothing was thrown. ./gradlew test --continue is green on the branch: 370 tests
on master, 378 on the branch.

Two existing tests changed with the view: the regression test for the last
absence now removes through clearDaysOff() instead of getDaysOff().clear(),
and the M/N test turned its "old route" half into the assertion that the old
route is now rejected.

HumanResource has addDaysOff() but no counterpart: the only way to remove an absence is to mutate the DefaultListModel handed out by getDaysOff(), and nothing observes that model. Adding resets the cached LoadDistribution and fires; removing does neither. The resource properties dialog clears the list before writing the edited intervals back. With intervals left the last addDaysOff() repairs both by accident; delete the LAST absence and no event is fired at all. Measured on screen: the absence keeps being drawn on the resource chart while the dialog's list is already empty. Fixed at the root rather than at the call site -- the resource now observes its own list, so every mutation through getDaysOff() resets the loads and notifies, including callers added later. addDaysOff() drops its own resetLoads()/fireResourceChanged() so that it does not fire twice. Safe for the copying constructor: areEventsEnabled is false there before the copy loop runs, and instance initialisers run before the constructor body, so the listener is installed and stays silent.

A day off could be given to a resource through addDaysOff(), but there was no way to take one away again: the only route was to reach into the DefaultListModel handed out by getDaysOff() and mutate it. That is what the resource properties dialog did, and it left HumanResource with a collection that has an entry point but no exit. Add the two missing counterparts, following the shape HumanResourceManager already uses for its own list of resources (add / remove / clear): boolean removeDaysOff(GanttDaysOff) -- take a single interval away void clearDaysOff() -- take all of them away removeDaysOff returns whether anything was removed, because that is also the answer to "did the listeners hear about it": the list stays silent when the interval was not there. Without the return value a caller would have to look into the handed-out list again, which is the thing being avoided. It is named clearDaysOff rather than plain clear() because a resource holds assignments and custom properties too. Matching is by Object.equals, the way the list itself matches. GanttDaysOff only overloads equals(GanttDaysOff) and does not override equals(Object), so an interval built afresh from the same two dates is not the one the resource holds. That is documented on the method and pinned by a test rather than changed here. GanttDialogPerson.applyChanges() now calls clearDaysOff() instead of getDaysOff().clear(). No production code writes to the handed-out list any more. The number of notifications does not change: clearing and then writing N intervals back still costs (M > 0 ? 1 : 0) + N resourceChanged events, because clearDaysOff() calls the very same DefaultListModel.clear() the dialog used to call itself. A test walks every combination of M and N in 0..3 through both routes and pins the absolute counts, so a future implementation that fires once per interval would be caught. getDaysOff() is deliberately left exactly as it was -- signature, return type and body. Narrowing it is a separate change; the list it hands out is still mutable, so the old route remains open to anyone who takes it.

dbarashev · 2026-09-01T10:38:43Z

@Natalie-the-technician maybe it is a good time to replace DefaultListModel with a standard List in the HumanResource::getDaysOff() return type and in the underlying field? DefaultListModel is a Swing class, and despite that it is a "model", it is actually a model of a UI control.

Natalie-the-technician · 2026-09-01T11:44:29Z

Happy to. I looked at what depends on it first, and there is one consequence worth
naming before I change it, because it decides the shape of the result.

DefaultListModel is indeed never used as a UI model here. GanttDialogPerson
copies the intervals into its own DateIntervalListEditor model rather than
binding to it, and the four other readers only iterate:

	uses
`GanttDialogPerson` (dialog setup)	`getSize()`, `get(i)`
`VacationSaver`	`size()`, `getElementAt(i)`
`LoadDistribution.processDaysOff`	`size()`, `get(i)`
`OverwritingMerger`	`size()`, `get(i)`
`HumanResource` copying constructor	`getSize()`, `get(i)`

No test touches getDaysOff(). So the switch itself is small.

The consequence: this PR fixes the missing notification by observing the list —
a ListDataListener on the DefaultListModel. A plain List cannot be observed,
so the notification has to move into addDaysOff / removeDaysOff /
clearDaysOff, and then the getter has to stop handing out something mutable.
Otherwise the defect comes straight back: a caller mutating the returned list would
once again change the resource without resetting myLoadDistribution and without
firing, which is exactly what this PR is about.

That is a better end state than what is in the PR now, and it makes the three
methods the only way in and out. It does change a public signature, which is why I
did not go there unasked.

Two things I would rather have your preference on than guess:

List<GanttDaysOff> or Collection<GanttDaysOff> as the return type?
Collections.unmodifiableList(new ArrayList<>(...)), the way
HumanResourceManager.getResources() does it at line 217 — or an unmodifiable
view without the copy?

I will fold it into this PR rather than open a second one, since without the
unmodifiable getter the type change would leave the hole open.

dbarashev · 2026-09-01T12:11:26Z

I believe it must be an unmodifiable view with the List type.

DefaultListModel is a Swing class: despite the name it models a UI control, not a resource. HumanResource::getDaysOff() now returns List<GanttDaysOff>, and the field behind it is a plain ArrayList with Collections.unmodifiableList over it. A view rather than a copy, so a caller holding on to it keeps seeing the resource. The notification had to move into the mutating methods. A DefaultListModel can be watched with a ListDataListener, which is how the removal of the last day off came to notify its listeners; a plain List cannot be watched. addDaysOff, removeDaysOff and clearDaysOff now reset the load distribution and fire themselves. That only holds because the getter turned tight at the same time: while the handed-out list was modifiable, a caller could change the resource without either of the two happening, which is the defect this branch fixes. The number of notifications is unchanged. removeDaysOff stays silent and returns false when nothing was removed, and clearDaysOff stays silent when the list was already empty -- exactly what DefaultListModel.removeElement() and clear() did. The resource properties dialog's clear-all-and-rewrite therefore still costs (M > 0 ? 1 : 0) + N notifications, and the test walking every M and N in 0..3 still pins those numbers. The reading callers move from getSize()/getElementAt(i) to size()/get(i); LoadDistribution and ProjectFileExporter read through a raw DefaultListModel and can drop their casts. The copying constructor fills the list directly instead of going through addDaysOff(), so copying stays silent without depending on the areEventsEnabled flag for the days off at all.

Natalie-the-technician · 2026-09-01T18:02:22Z

Done — getDaysOff() now returns an unmodifiable List view.

The field behind it is a plain ArrayList, and getDaysOff() hands out
Collections.unmodifiableList over it, so callers keep seeing the resource but
cannot change it. The reading callers moved from getSize()/getElementAt(i) to
size()/get(i); LoadDistribution and ProjectFileExporter were reading
through a raw DefaultListModel and lost their casts along the way.

Two places where this was more than a type swap:

The notification moved into the methods. A DefaultListModel could be
watched with a ListDataListener, which is how the removal of the last day off
came to notify its listeners; a plain List cannot be watched. addDaysOff,
removeDaysOff and clearDaysOff now reset the load distribution and fire
themselves. That is only sound because the getter turned tight at the same time
— while the handed-out list was modifiable, a caller could change the resource
without either happening, which is the defect this PR fixes.
The event count is unchanged. DefaultListModel.clear() does not fire on an
empty list and removeElement() does not fire when nothing matched; List has
no such behaviour, so clearDaysOff() returns early when the list is already
empty and removeDaysOff() returns false without firing. The dialog's
clear-all-and-rewrite therefore still costs (M > 0 ? 1 : 0) + N events, and
the test walking every M and N in 0..3 still pins those absolute numbers.

Two new tests pin the return value itself: that modifying it throws
UnsupportedOperationException, and that it is a view rather than a copy (a day
off added after the call shows up in a list fetched before it). The first one
fails against the previous state of this branch with
Expected java.lang.UnsupportedOperationException to be thrown, but nothing was thrown.

./gradlew test --continue is green: 376 tests before, 378 after, the two new
ones being the difference.

dbarashev

Looks good. A few relatively minor comments below.

dbarashev · 2026-09-01T20:17:10Z

      if (p.getDaysOff() != null)
        for (int j = 0; j < p.getDaysOff().size(); j++) {
-          GanttDaysOff gdo = (GanttDaysOff) p.getDaysOff().getElementAt(j);
+          GanttDaysOff gdo = p.getDaysOff().get(j);


Can you please save days off into a local variable, to avoid calling it (and creating a read-only view) many times?

dbarashev · 2026-09-01T20:29:41Z

Any sense to join these three test classes into a single one?

dbarashev · 2026-09-01T20:46:11Z

    });

-    person.getDaysOff().clear();
+    person.clearDaysOff();


maybe we can introduce setDaysOff() method which would replace the entire list (and send just one notification) ?

dbarashev · 2026-09-01T20:46:59Z

  private void processDaysOff(HumanResource resource) {
-    DefaultListModel daysOff = resource.getDaysOff();
+    List<GanttDaysOff> daysOff = resource.getDaysOff();
    if (daysOff != null) {


This is redundant now

dbarashev · 2026-09-01T20:47:19Z

@@ -36,7 +36,7 @@ void save(IGanttProject project, TransformerHandler handler) throws SAXException
    for (HumanResource p : project.getHumanResourceManager().getResources()) {
      if (p.getDaysOff() != null)


No more needed

dbarashev · 2026-09-01T20:48:43Z

+   *
+   * @return true if the interval was there and has been removed, false if there was nothing to do
+   */
+  public boolean removeDaysOff(GanttDaysOff gdo) {


This method is not yet used, right? Do we have any plans of using it?
Anyway, since we delegate removal to the list, it makes sense to define equals and hashCode methods in GanttDaysOff

Natalie-the-technician added 2 commits August 31, 2026 11:38

dbarashev reviewed Sep 1, 2026

View reviewed changes

Navigation Menu

Uh oh!

Notify listeners when days off are removed, and add the missing removal methods - #2830

Notify listeners when days off are removed, and add the missing removal methods#2830
Natalie-the-technician wants to merge 3 commits into
bardsoftware:masterfrom
Natalie-the-technician:daysoff-removal-notifies

Natalie-the-technician commented Aug 31, 2026 •

edited

Loading

Uh oh!

dbarashev commented Sep 1, 2026

Uh oh!

Natalie-the-technician commented Sep 1, 2026

Uh oh!

dbarashev commented Sep 1, 2026

Uh oh!

Natalie-the-technician commented Sep 1, 2026

Uh oh!

dbarashev left a comment

Uh oh!

dbarashev Sep 1, 2026

Uh oh!

dbarashev Sep 1, 2026

Uh oh!

dbarashev Sep 1, 2026

Uh oh!

dbarashev Sep 1, 2026

Uh oh!

dbarashev Sep 1, 2026

Uh oh!

dbarashev Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -36,7 +36,7 @@ void save(IGanttProject project, TransformerHandler handler) throws SAXException
		for (HumanResource p : project.getHumanResourceManager().getResources()) {
		if (p.getDaysOff() != null)

Uh oh!

Conversation

Natalie-the-technician commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is wrong

Steps to reproduce

Cause

What this changes

The number of notifications does not change

Tests

Uh oh!

dbarashev commented Sep 1, 2026

Uh oh!

Natalie-the-technician commented Sep 1, 2026

Uh oh!

dbarashev commented Sep 1, 2026

Uh oh!

Natalie-the-technician commented Sep 1, 2026

Uh oh!

dbarashev left a comment

Choose a reason for hiding this comment

Uh oh!

dbarashev Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

dbarashev Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

dbarashev Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

dbarashev Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

dbarashev Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

dbarashev Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Natalie-the-technician commented Aug 31, 2026 •

edited

Loading